fix(tests): gate on firmware capability, not host bindings or source text - #52
Merged
Merged
Conversation
…ce text Two gates were checking something other than what they claimed, and both went green on a branch where the thing they gate was absent. requires_message() asks whether python-keepkey's OWN bindings define a message. That is a property of the pinned submodule, not of the firmware under test, so it passes on every branch regardless. The structured EIP-712 suite used it, and on feat/passkeys-7.16 -- which has no eip712_stream.c at all -- four tests failed as though the feature were broken rather than absent. Replaced with requires_structured_eip712(), which probes the device: firmware without the walk answers the opening message with Failure_UnexpectedMessage. Any OTHER failure deliberately does NOT skip, because "present but misbehaving" must never be mistaken for "absent" -- that is how a skipped test becomes a silent pass. test_burned_versions_have_no_reader asserted the ABSENCE of a `case StorageVersion_18:` label, reasoning that falling to the default is what sends a burned format to the wipe path. There is no default: storage_fromFlash omits one deliberately so -Werror=switch names any version we forget. So an unlisted version does not fall anywhere, it breaks the ARM build -- which is exactly what happened. Now asserts the real property: the labels exist, and what they dispatch to is SUS_Invalid with no storage_readVxx behind them. Verified the test is not vacuous by injecting a reader and watching it fail.
BitHighlander
added a commit
to BitHighlander/keepkey-firmware
that referenced
this pull request
Aug 22, 2026
Picks up BitHighlander/python-keepkey#52, which fixes the two gates that failed this branch: - the structured EIP-712 suite gated on requires_message(), which only asks whether python-keepkey's OWN bindings define a message. That is a property of the pinned submodule, not the firmware, so it passed here despite this branch having no eip712_stream.c at all, and four tests failed as though the feature were broken rather than absent. Now probes the device. - test_burned_versions_have_no_reader asserted the ABSENCE of a `case StorageVersion_18:` label. storage_fromFlash has no default case on purpose, so -Werror=switch requires that label to exist -- the test and the compiler gate were in direct contradiction. Now asserts the property that matters: the labels dispatch to SUS_Invalid with no reader behind them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two gates checked something other than what they claimed, and both went green on a branch where the thing they gate is absent.
requires_message()can never failIt asks whether python-keepkey's own bindings define a message. That is a property of the pinned submodule, not of the firmware under test — so it passes on every branch regardless of what the device implements.
The structured EIP-712 suite relied on it. On
feat/passkeys-7.16, which has noeip712_stream.cat all, four tests failed as though the feature were broken rather than absent.Replaced with
requires_structured_eip712(), which probes the device. Firmware without the walk answers the opening message withFailure_UnexpectedMessage.Deliberately, any other Failure does not skip — the test runs and reports. "Present but misbehaving" must never be mistaken for "absent"; that is how a skipped test becomes a silent pass, which has already cost us once.
test_burned_versions_have_no_readerasserted the wrong thingIt asserted the absence of a
case StorageVersion_18:label, on the theory that falling through to the default is what sends a burned format to the wipe path.There is no default.
storage_fromFlashomits one deliberately so that-Werror=switchnames any version we forget. So an unlisted version does not fall anywhere — it breaks the ARM build, which is exactly what happened on the passkey branch.The labels must exist. What must not exist is a reader behind them. The test now asserts that real property: 18 and 19 are dispatched, and what they dispatch to is
SUS_Invalidwith nostorage_readVxxcall in the arm.Verified not vacuous — injecting
storage_readV17behind the burned labels makes it fail:Verification
test_storage_version_gate.py: 5 passed, 4 skipped, run locally. The EIP-712 gate needs a device and is exercised by firmware CI.